Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Metal LB addon #7308

Merged
merged 6 commits into from
Apr 24, 2020
Merged

Add Metal LB addon #7308

merged 6 commits into from
Apr 24, 2020

Conversation

laozc
Copy link
Contributor

@laozc laozc commented Mar 29, 2020 •
edited
Loading

This PR configures Metal LB for an ARP based Load Balancer in local network.
This addon was verified with both Hyper-V and KVM driver.

Before using the addons, you may need to use minikube ip to get the details for the subnet to find out which IP addresses can be used for the LB to avoid IP address collision.

$ minikube ip
192.168.39.225

You may then use the following command to configure and enable the addon.

$ minikube addons configure metallb
-- Enter Load Balancer Start IP: 192.168.39.100
-- Enter Load Balancer End IP: 192.168.39.200
✅  metallb was successfully configured
$ ./minikube addons enable metallb
✅  The 'metallb' addon is enabled

Then you may create a service with type LoadBalancer.

---
apiVersion: v1
kind: Service
metadata:
  name: nginx
  namespace: default
  labels:
    app: nginx
spec:
  ports:
  - name: http
    port: 80
    protocol: TCP
    targetPort: 80
  selector:
    app: nginx
  type: LoadBalancer

Eventually the service will obtain an IP.

$ kubectl get service
NAME         TYPE           CLUSTER-IP      EXTERNAL-IP      PORT(S)        AGE
kubernetes   ClusterIP      10.96.0.1       <none>           443/TCP        117m
nginx        LoadBalancer   10.103.15.133   192.168.39.100   80:30063/TCP   113m

You may access the External IP on your host.

$ curl 192.168.39.100:80
<!DOCTYPE html>
<html>
<head>
<...
</html>

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 29, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @laozc. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: laozc
To complete the pull request process, please assign josedonizetti
You can assign the PR to them by writing /assign @josedonizetti in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@laozc laozc changed the title Add Metal LB addons Add Metal LB addon Mar 29, 2020
@codecov-io
Copy link

codecov-io commented Mar 29, 2020 •
edited
Loading

Codecov Report

Merging #7308 into master will decrease coverage by 0.65%.
The diff coverage is 3.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7308      +/-   ##
==========================================
- Coverage   38.02%   37.37%   -0.66%     
==========================================
  Files         147      146       -1     
  Lines        9017     8894     -123     
==========================================
- Hits         3429     3324     -105     
- Misses       5164     5165       +1     
+ Partials      424      405      -19
Impacted Files Coverage Δ
cmd/minikube/cmd/config/configure.go 1.50% <0.00%> (-0.18%) ⬇️
cmd/minikube/cmd/config/prompt.go 13.41% <0.00%> (-2.08%) ⬇️
cmd/minikube/cmd/start.go 36.21% <0.00%> (+0.07%) ⬆️
pkg/minikube/cluster/pause.go 0.00% <ø> (ø)
pkg/minikube/machine/cache_images.go 0.00% <0.00%> (ø)
pkg/minikube/problem/err_map.go 100.00% <ø> (ø)
cmd/minikube/cmd/pause.go 11.11% <100.00%> (ø)
cmd/minikube/cmd/unpause.go 10.00% <100.00%> (ø)
pkg/minikube/machine/machine.go 0.00% <0.00%> (-28.27%) ⬇️
pkg/minikube/registry/global.go 58.69% <0.00%> (-22.56%) ⬇️
... and 22 more

tstromberg
tstromberg reviewed Mar 29, 2020
View reviewed changes
Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addon looks good.

I'm going to need some time to think about where the new flags go. It doesn't feel right to have an addon specific flag in "start".

Could the value be auto-configured or attached to the addon or config command somehow?

@laozc
Copy link
Contributor Author

laozc commented Mar 29, 2020 •
edited
Loading

It would be difficult to detect the right values for the load balancer IP since it may vary in the user's own network environment.
Removing those flags from start should be fine as long as the user uses configure to set the start/end IP.

@medyagh
Copy link
Member

medyagh commented Mar 29, 2020

I have been thinking about metallb for ingress or load balancer ( alternative to tunnel)

@laozc do you mind cleariying which driver have you tested this add-on with ?
And does it work on docker driver on linux and also docker driver on macOs

@laozc
Copy link
Contributor Author

laozc commented Mar 29, 2020

I'm testing it with Hyper-V and KVM drivers and both work fine.
You can access the assigned service IP without tunneling since the service IP is bound to the MAC address by Metal LB.
I haven't done any test on macOS nor the docker driver yet and not sure if it would work on these scenarios.

@medyagh
Copy link
Member

medyagh commented Mar 29, 2020

sting it with Hyper-V and KVM drivers and both work fine.
You can access the assigned service IP without tunneling since the service IP is bound to the MAC address by Metal LB.
I haven't done any test on macOS nor the docker driver yet a

do u mind also putting an example of the metal lb in the PR description ?

and how would this be different than our current minikube tunnel command ?

@laozc
Copy link
Contributor Author

laozc commented Mar 30, 2020

The major change is that you don't need to run minikube tunnel.
Metal LB sets the ARP table for the exposed service to ensure the service IP is accessible from your machine.

You may check on the updated PR description.

tstromberg
tstromberg suggested changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@tstromberg tstromberg left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding more start flags, is it possible to change this PR to use minikube addons configure metallb? See registry-creds as an example.

(edit: nevermind, I see you have now done that!)

tstromberg
tstromberg reviewed Apr 9, 2020
View reviewed changes
pkg/minikube/config/types.go Outdated
FeatureGates string // https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/
ServiceCIDR string // the subnet which kubernetes services will be deployed to
ImageRepository string
LoadBalancerStartIP string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment that the LoadBalancer IP's are currently only used by MetalLB

@tstromberg
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 10, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [66.379136058 67.10636131 69.224124656]
Average time for minikube: 67.569874008

Times for Minikube (PR 7308): [66.72947534900001 67.132664835 67.15591933799999]
Average time for Minikube (PR 7308): 67.00601984066667

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 7308) |
+--------------------------------+-----------+--------------------+
| * minikube v1.9.2 on Debian    |  0.060552 |           0.060230 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.020743 |           0.019869 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.003107 |           0.003087 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 41.965960 |          41.630275 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.0 | 23.456248 |          22.924746 |
| on Docker 19.03.8 ...          |           |                    |
| * Enabling addons:             |  1.975542 |           2.277741 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.083694 |           0.085360 |
| configured to use "minikube"   |           |                    |
|                                |  0.004029 |           0.004711 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [47.179769363000005 64.511409377 47.62610184099999]
Average time for minikube: 53.10576019366666

Times for Minikube (PR 7308): [47.583713524 48.404982374999996 47.210017097999994]
Average time for Minikube (PR 7308): 47.732904332333334

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 7308) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.9.2 on Debian            |  0.076640 |           0.076071 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.002352 |           0.002332 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.000090 |           0.000143 |
| minikube in cluster minikube           |           |                    |
| * Pulling base image ...               |  0.180468 |           0.177909 |
| * Creating Kubernetes in               | 10.507599 |          10.710797 |
| docker container with (CPUs=2)         |           |                    |
| (4 available), Memory=3700MB           |           |                    |
| (15043MB available) ...                |           |                    |
| * Preparing Kubernetes v1.18.0         |  0.133216 |           0.107529 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 34.601817 |          35.106662 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Enabling addons:                     |  7.506408 |           1.471673 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.092501 |           0.076475 |
| configured to use "minikube"           |           |                    |
|                                        |  0.004667 |           0.003314 |
+----------------------------------------+-----------+--------------------+

@tstromberg tstromberg merged commit e3a3b1e into kubernetes:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tstromberg tstromberg tstromberg approved these changes

@blueelvis blueelvis Awaiting requested review from blueelvis

@josedonizetti josedonizetti Awaiting requested review from josedonizetti

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@laozc @k8s-ci-robot @minikube-bot @codecov-io @medyagh @tstromberg @minikube-pr-bot